Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(entgql): check fields RType for Map and use Map scalar if possible #455

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

tankbusta
Copy link
Contributor

@tankbusta tankbusta commented Mar 14, 2023

If you create an entity with a JSON field...

type Example struct {
	ent.Schema
}

func (Example) Fields() []ent.Field {
	return []ent.Field{
		field.JSON("metadata", map[string]interface{}{}).
			Optional(),
	}
}

You will get an error when running code generation

2023/03/13 19:21:39 running ent codegen: field(metadata): entgql: json type not implemented

You can resolve this today by adding the annotation entgql.Type("Map") but I didn't see any mention that this was required so I made the code a little bit smarter by checking if it's of map[string]interface {} and using the built-in Map scalar. If you do not set this, the error is made smarter by recommending that you set entgql.Type.

@a8m
Copy link
Member

a8m commented Mar 15, 2023

Thanks for the contribution, @tankbusta 🙏🏻

Can you add a simple test for this? A field in one of these schemas: /~https://github.com/ent/contrib/tree/master/entgql/internal/todo/ent/schema

@tankbusta tankbusta force-pushed the fix/entgql_json_type branch from cce019e to 21984a2 Compare March 17, 2023 23:44
@giautm giautm self-assigned this Mar 18, 2023
Copy link
Member

@a8m a8m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@@ -773,6 +773,13 @@ func (e *schemaGenerator) mapScalar(gqlType string, f *gen.Field, ant *Annotatio
case "[]string":
scalar = "[String!]"
}
case reflect.Map:
if f.Type.RType.Ident == "map[string]interface {}" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spacing between interface and {} really weird. But the test passed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was confused when I saw that at first but I think it's just how the reflect package identifies interface{}. You can see the same format in some of their tests: /~https://github.com/golang/go/blob/master/src/reflect/all_test.go#L713

@giautm giautm force-pushed the fix/entgql_json_type branch from b52fa06 to 5323245 Compare March 24, 2023 21:41
@giautm giautm merged commit bb1039b into ent:master Mar 24, 2023
@giautm
Copy link
Collaborator

giautm commented Mar 24, 2023

Thank @tankbusta for your contribution. Good to see your here again, <3

@tankbusta tankbusta deleted the fix/entgql_json_type branch March 27, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants